External staking locks 2 #60
Conversation
maurolacy
force-pushed
the
external-stacking-locks
branch
from
73c8eea
to
8fd2445
Compare
maurolacy
force-pushed
the
external-staking-locks-2
branch
3 times, most recently
from
132606f
to
639873f
Compare
There was a problem hiding this comment.
Looks quite good.
A few missing TODOs I commented on, and some minor code nits.
Main point is removing lock on distribution.
And then exposing these commit/rollback methods as SudoMsg for testing.
If you can fix this up, I can work on the corresponding side for converter (and write the external-staking ack handlers as well if you want... just pull out the logic for an easy call)
| @@ -451,6 +451,8 @@ fn get_last_pending_tx_id(vault: &VaultContractProxy) -> Option<u64> { | |||
| let txs = vault.all_pending_txs_desc(None, None).unwrap().txs; | |||
| txs.first().map(|tx| match tx { | |||
| Tx::InFlightStaking { id, .. } => *id, | |||
| Tx::InFlightRemoteStaking { id, .. } => *id, | |||
| Tx::InFlightRemoteUnstaking { id, .. } => *id, | |||
There was a problem hiding this comment.
Nit. I think it is easier to not include the other enum now.
And only add it when you want to implement it, so the compiler shows you all the places that need the updates
(easier than searching for unimplemented!())
| /// Remote validator | ||
| validator: String, | ||
| }, | ||
| // IBC flight |
There was a problem hiding this comment.
Better rustdoc here and above?
| } => { | ||
| write!( | ||
| f, | ||
| "InFlightRemoteStaking {{ id: {}, amount: {}, user: {}, validator: {} }}", |
There was a problem hiding this comment.
This is basically the Debug format, right?
I would assume Display to do something more custom. Or just delegate to Debug.
There was a problem hiding this comment.
Yes, simplifying this to InFlightRemoteStaking {{ id: {}}} makes sense.
Add tx type verification
ethanfrey
force-pushed
the
external-staking-locks-2
branch
from
1a96907
to
34c0f99
Compare
| } | ||
|
|
||
| // put this later so compiler doens't complain about mut in test mode | ||
| resp = resp.add_attribute("owner", ctx.info.sender); |
There was a problem hiding this comment.
Nit: Move all the add_attribute calls here.
There was a problem hiding this comment.
I refer to amount.amount which is used in the conditionally compiled block, so moving would force an unnecessary clone.
Kinda annoying to make clippy happy on both branches (test / not(test))
Happy for your cleanup on this later
| /// Must be called by the IBC callback handler on failed remote staking. | ||
| #[msg(exec)] | ||
| fn test_rollback_stake(&self, ctx: ExecCtx, tx_id: u64) -> Result<Response, ContractError> { | ||
| #[cfg(test)] |
There was a problem hiding this comment.
The pattern in converter, in which you call an internal method here for tests and do nothing otherwise is better / clearer. I'll think you'll refactor to that, when connecting with the IBC handlers.
There was a problem hiding this comment.
Yup, I just refactored to that, working on one step at a time to keep tests passing.
#57 follow-up. Implementation of staking locks for the external-staking contract.
TODO: